Skip to content

Remove user from project before deletion #10008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Dec 4, 2024
Merged

Remove user from project before deletion #10008

merged 3 commits into from
Dec 4, 2024

Conversation

@winterhazel
Copy link
Member

@winterhazel winterhazel commented Nov 28, 2024

Description

If a user that is associated to a project gets deleted, other users become unable to see who belongs to the project via listProjectAccounts due to a NPE. This happens because the removed user is still associated with the project.

This PR fixes the issue by removing the user from all projects before deleting it (as we already do with accounts). To normalize environments that are affected by the bug, the following script will be executed during the upgrade in order to delete project-user associations for users that were removed.

DELETE FROM `cloud`.`project_account` WHERE `user_id` IN (SELECT `id` FROM `cloud`.`user` WHERE `removed`);

Fixes #9974

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

User clean-up on deletion

  1. I created a project
  2. I added a user to the project
  3. I deleted the user

After step 3, I would become unable to list accounts/users that belong to the project via listProjectAccounts before the changes. With the changes, I was able to list them normally.

Normalization of affected environments

Before applying the patch, I created some broken projects by deleting their users. Then, I executed the upgrade script and verified that I was able to list their accounts/users normally.

@winterhazel winterhazel added this to the 4.19.2 milestone Nov 28, 2024
@winterhazel winterhazel requested a review from rajujith November 28, 2024 20:27
@codecov
Copy link

codecov bot commented Nov 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 22.50000% with 31 lines in your changes missing coverage. Please review.

Project coverage is 15.12%. Comparing base (a2690e9) to head (414cc6e).
Report is 9 commits behind head on 4.19.

Files with missing lines Patch % Lines
...ava/com/cloud/upgrade/dao/Upgrade41910to41920.java 15.38% 22 Missing ⚠️
.../com/cloud/projects/dao/ProjectAccountDaoImpl.java 0.00% 7 Missing ⚠️
...c/main/java/com/cloud/user/AccountManagerImpl.java 66.66% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               4.19   #10008      +/-   ##
============================================
+ Coverage     15.10%   15.12%   +0.01%     
- Complexity    11225    11256      +31     
============================================
  Files          5404     5408       +4     
  Lines        473502   473838     +336     
  Branches      57733    57770      +37     
============================================
+ Hits          71543    71672     +129     
- Misses       393961   394168     +207     
  Partials       7998     7998
Flag Coverage Δ
uitests 4.30% <ø> (-0.01%) ⬇️
unittests 15.84% <22.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DaanHoogland
DaanHoogland approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

sureshanaparti
sureshanaparti approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments, overall clgtm

winterhazel and others added 2 commits November 29, 2024 10:44
@winterhazel @sureshanaparti
Apply suggestions from code review
4fc63f9 
Co-authored-by: Suresh Kumar Anaparti <[email protected]>
@winterhazel @sureshanaparti
Apply suggestions from code review
414cc6e 
Co-authored-by: Suresh Kumar Anaparti <[email protected]>
@sureshanaparti
Copy link
Contributor

sureshanaparti commented Nov 29, 2024

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Nov 29, 2024

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Nov 29, 2024

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11657

rajujith
rajujith approved these changes Dec 2, 2024
View reviewed changes
Copy link

@rajujith rajujith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Tested, this PR fixed the reported issue.

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Dec 3, 2024

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Dec 3, 2024

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

blueorangutan commented Dec 3, 2024

[SF] Trillian test result (tid-11835)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 45441 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr10008-t11835-kvm-ol8.zip
Smoke tests completed. 132 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_secure_vm_migration Error 134.37 test_vm_life_cycle.py
test_01_secure_vm_migration Error 134.37 test_vm_life_cycle.py

@DaanHoogland DaanHoogland merged commit ef1a58d into apache:4.19 Dec 4, 2024
25 of 26 checks passed
DaanHoogland added a commit that referenced this pull request Dec 4, 2024
@DaanHoogland
Merge release branch 4.20 to main
205ebfb 
* 4.20:
  UI: Fix userdata and load balancer selection (#10016)
  Prevent password updates for SAML and LDAP users (#9999)
  cloudstack-migrate-databases: sql AND added (#10033)
  engine/schema: move SQLs to 4.20.0 to 4.20.1 upgrade (#10018)
  Remove user from project before deletion (#10008)
  Simplify validation for creating volume templates via UI (#9828)
@DaanHoogland DaanHoogland mentioned this pull request Dec 4, 2024
Closed
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Dec 12, 2024
@winterhazel @sureshanaparti @dhslove
Remove user from project before deletion (apache#10008)
68d247d 
Co-authored-by: Suresh Kumar Anaparti <[email protected]>
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Dec 12, 2024
@DaanHoogland @dhslove
Merge release branch 4.20 to main
58a4079 
* 4.20:
  UI: Fix userdata and load balancer selection (apache#10016)
  Prevent password updates for SAML and LDAP users (apache#9999)
  cloudstack-migrate-databases: sql AND added (apache#10033)
  engine/schema: move SQLs to 4.20.0 to 4.20.1 upgrade (apache#10018)
  Remove user from project before deletion (apache#10008)
  Simplify validation for creating volume templates via UI (apache#9828)
@winterhazel winterhazel mentioned this pull request Dec 17, 2024
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@rajujith rajujith rajujith approved these changes

@Pearl1594 Pearl1594 Pearl1594 approved these changes

@sureshanaparti sureshanaparti sureshanaparti approved these changes

Assignees

No one assigned

Labels

component:projects

Projects

None yet

Milestone

4.19.2

Development

Successfully merging this pull request may close these issues.

6 participants

@winterhazel @sureshanaparti @blueorangutan @DaanHoogland @rajujith @Pearl1594